fix(controller)!: do not attempt to infer desired revision when not specified #2980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
==========================================
+ Coverage 51.04% 51.08% +0.03%
==========================================
Files 280 282 +2
Lines 25380 25365 -15
==========================================
+ Hits 12955 12957 +2
+ Misses 11719 11709 -10
+ Partials 706 699 -7 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
krancour
changed the title
hiddeco
approved these changes
Nov 22, 2024
Actually looks like codegen verification just timed out in CI... context deadline exceeded. Kicked it and it ran to completion. |
fykaa
pushed a commit
to fykaa/kargo
that referenced
this pull request
Dec 20, 2024
…pecified (akuity#2980) Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2952 #2968
When determining whether an Application sync has completed successfully and again when assessing Application health as a factor in Stage health, the argocd-update step (as did legacy promotion mechanisms before it) often takes revision information into account. i.e. It often requires ApplicationSources to be observably synced to a specific revision (commit ID or chart version).
Until now, when the desired revision for a given ApplicationSource has not been explicitly specified, the argocd-update step has attempted to infer the desired revision by examining the Promotion's FreightCollection. If it found an artifact matching an ApplicationSource's repoURL field (and chart field, when applicable), that revision of the artifact was presumed to be the one that ApplicationSource should be observably synced to.
This behavior has repeatedly tripped up users for a variety of reasons, the most prominent involving a scenario wherein multiple Stages use promotion processes that update the head of a single, common branch, which is tracked by multiple Applications. When a Promotion to one such Stage pushes a new commit, deemed its "desired revision," any Application that tracks that branch (and subsequently syncs) will no longer observably be synced to its own "desired revision." A Stage's health checks count this apparent mis-sync as a health problem.
In other scenarios, this behavior has also been implicated in causing the argocd-update step to enter an infinite series of retries for what appear to be failed attempts to sync an Application.
This PR simply eliminates the behavior whereby the argocd-update step attempts to infer desired revision when none is specified. This essentially relegates the step's desiredRevision field (and the deprecated desiredCommitFromStep field) to the status of an "advanced" feature for use in scenarios not prone to the difficulties highlighted above.
This is a breaking change insofar as it changes behavior, although it will not actually break anything as it relaxes success criteria. Most users will notice nothing different. It is only users who like the more thorough health checks who may wish to now begin explicitly specifying desired revisions, which is something easily accomplished using v1.1's new EL support.
This change will be prominently mentioned in the v1.1.0 release notes.